Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(fix) Computer feature: update removable devices on Linux #12893

Merged
merged 4 commits into from
Apr 14, 2024
Merged

(fix) Computer feature: update removable devices on Linux #12893

merged 4 commits into from
Apr 14, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Feb 28, 2024 •
edited
Loading

Re #12891

Issue 1

I didn't see my SD card in /media/user after I re-mounted it.
I have been focused only on /media/[user] because this is where my USB/SD drives are mounted. Devices changes (or removed and re-mounted devices) where not reflected in the tree view because these are subdirs of the device lookup path /media for which the respective 'hasChildren()` state has been cached.

This is not the case for 1st-level children of the lookup paths, e.g. /run/media/[user]. In those we show devices correctly (newly added or removed) when (re-)expanding the 'Removable Devices' node.

Fix:
add /media/[user] to lookup paths, but don't show it when adding children of /media ✔️
= all devices are now leaves of the Devices branch

Issue 2

We won't see changes in the dir tree after initial expand due to hasChildren() caching.
While for static mounts this may be an okayish tradeoff for (assumed) faster repaint for static mounts, it is much more likely that the dir tree of a removable device has changed.

Ideas and simple benchmarking:
a) when expanding Devices, tell the FolderTreeitemModel to remove all child paths from the cache:
with 10.000 cached paths this takes about 2.5ms, and is only done once when [devices] is expanded
b) in directoryHasChildren, check if path starts with one of the device root paths, if yes: don't cache hasChildren
with 10.000 cached paths this takes about 7ms per dir, and causes some notable delay under some circumstances

Fix:
So the winner is a) ✔️
To refresh, just collapse & expand 'Removable Devices' (click on branch icon or hit Return twice).

Addon / Proposal

We could add a new Browse item action 'Refresh directory tree', mapped to the new method FolderTreeModel::removeChildDirsFromCache(QStringList).
This would allow to force-refresh the dir tree of that item, e.g. when you copied dirs to a previously empty QuickLink dir.
#12908

TODO
make Refresh more discoverable (tooltip), mention it in the manual

Related meta discussion: https://mixxx.zulipchat.com/#narrow/stream/109122-general/topic/linux.3A.20mounting.20drive.20after.20starting.20mixxx

@ronso0 ronso0 force-pushed the browse-removable-devices-update branch 2 times, most recently from 13cebcc to 6349bb7 Compare February 29, 2024 00:22
@ronso0 ronso0 linked an issue Feb 29, 2024 that may be closed by this pull request
'Computer' sidebar is not updated correctly when removeable drives are un/mounted #12891
Closed
@ronso0 ronso0 marked this pull request as ready for review February 29, 2024 01:22
acolombier
acolombier reviewed Feb 29, 2024
View reviewed changes
src/library/browse/browsefeature.cpp Outdated
const QStringList linuxRemovableDrivePaths() {
QStringList paths;
paths.append("/media");
paths.append(QStringLiteral("/run/media/") + QString::fromLocal8Bit(qgetenv("USER")));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be the same for /media on Ubuntu/Pop-Os - /media/$USER

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, it's like that on Ubuntu (which Pop!OS is based on).
Though TLDP https://tldp.org/LDP/Linux-Filesystem-Hierarchy/html/media.html says (can't tell how up-to-date that is, and which distros have their own rules, but /media seems to be the common base):

The following directories, or symbolic links to directories, must be in /media, if the corresponding subsystem is installed:

floppy Floppy drive (optional)
cdrom CD-ROM drive (optional)
cdrecorder CD writer (optional)
zip Zip drive (optional)

On systems where more than one device exists for mounting a certain type of
media, mount directories can be created by appending a digit to the name of
those available above starting with '0', but the unqualified name must also
exist.

A compliant implementation with two CDROM drives might have /media/cdrom0
and /media/cdrom1 with /media/cdrom a symlink to either of these.

@ronso0

This comment was marked as outdated.

Sign in to view
@ronso0 ronso0 marked this pull request as draft March 1, 2024 01:47
@ronso0 ronso0 force-pushed the browse-removable-devices-update branch 2 times, most recently from 409f382 to 0a25887 Compare March 3, 2024 22:21
@ronso0 ronso0 marked this pull request as ready for review March 3, 2024 22:21
@ronso0 ronso0 mentioned this pull request Mar 3, 2024
Merged
@ronso0 ronso0 force-pushed the browse-removable-devices-update branch from 0a25887 to e6e66e1 Compare March 3, 2024 22:56
@ronso0 ronso0 force-pushed the browse-removable-devices-update branch from e6e66e1 to f73c8b1 Compare March 3, 2024 23:22
@ronso0 ronso0 added this to the 2.4.1 milestone Mar 4, 2024
@ronso0 ronso0 changed the title (fix)Computer feature: update removable devices (fix) Computer feature: update removable devices on Linux Mar 9, 2024
@ronso0
Copy link
Member Author

ronso0 commented Mar 20, 2024

Oh, one issue with moving [$USER] subdirs to the top level:
even with no devices directoryHasChildren returns true for Removable Devices (▶ icon) because it finds empty [$USER] but BrowseFeature doesn't display this directory anymore, so expanding the branch gives no results.

I'll check if that can be fixed without hacks, else I'll revert the respective commit.

@ronso0
Copy link
Member Author

ronso0 commented Mar 20, 2024 •
edited
Loading

Oh, one issue with moving [$USER] subdirs to the top level:
even with no devices directoryHasChildren returns true for Removable Devices (▶ icon) because it finds empty [$USER] but BrowseFeature doesn't display this directory anymore, so expanding the branch gives no results.

Okay, this is not a regression: I tested 2.4 after removing /media/USER and it's the same.

This is ready for merge.
Anybody wants to test?

@acolombier
Copy link
Member

acolombier commented Mar 22, 2024 •
edited
Loading

Tested and working as expected!

I have some custom mount points configured with udisk which aren't shown in Mixxx but this is expected due to the static path lists in removableDriveRootPaths().

(Not for this PR but for a next iteration) - what do you think of using /proc/mounts to detect removable devices on Linux? Paired with a partition type filter + QFileInfo::isReadable, I think it could work a treat and support my usecase, as well as those distros that comes with different mounting path.

@ronso0
Copy link
Member Author

ronso0 commented Mar 22, 2024

Sure, might work. df might also work.
Though I'll leave further tweaking to others ; )

@daschuer
Copy link
Member

Thank you @ronso0 for the PR and @acolombier for testing!

@daschuer daschuer merged commit 27e49ee into mixxxdj:2.4 Apr 14, 2024
14 checks passed
@ronso0 ronso0 deleted the browse-removable-devices-update branch April 14, 2024 20:28
@ronso0
Copy link
Member Author

ronso0 commented Apr 14, 2024

@acolombier Wanna file an issue re: /proc/mounts/df?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acolombier acolombier acolombier approved these changes

Assignees
No one assigned
Labels
browse library needs review
Projects
None yet
Milestone
2.4.1
Development

Successfully merging this pull request may close these issues.

'Computer' sidebar is not updated correctly when removeable drives are un/mounted
3 participants
@ronso0 @acolombier @daschuer